-
Notifications
You must be signed in to change notification settings - Fork 4
feat(artifacts): verification #82
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Reviewer's GuideThis PR drafts the artifact verification UI by overhauling the ArtifactResults component: it replaces simple layout with PatternFly MultiContentCard for summary, DataList for signature listings, expandable CodeBlock and TreeView for certificate chains, and clipboard interactions, while also adjusting the sidebar link text and setting a default artifact URI. Class diagram for updated ArtifactResults componentclassDiagram
class ArtifactResults {
+artifact: ImageMetadataResponse
+activeItems: TreeViewDataItem[]
+sigDropdownOpen: Record<string, boolean>
+expanded: string[]
+copied: boolean
+summaryCards: Card[]
+options: TreeViewDataItem[]
+clipboardCopyFunc(text)
+onClick(text)
+onSelectCertChain(event, treeViewItem)
+setSigDropdownState(id, isOpen)
+handleOpenSigDropdown(id)
+handleToggleClick(id)
+handleSelect(id)
+toggle(id)
+codeBlockActions
+render()
}
ArtifactResults --> ImageMetadataResponse
ArtifactResults --> TreeViewDataItem
ArtifactResults --> Card
Class diagram for updated Artifacts page stateclassDiagram
class Artifacts {
-artifactUri: string | null = "docker.io/library/nginx:latest"
-labelHelpRef
}
Class diagram for updated SidebarApp navigationclassDiagram
class SidebarApp {
+render()
"Trust Root" link text updated
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
feat(artifacts): update mock and query feat(artifact): add form control and field validation fix: re-add hook state fix(ci): type issues with react-syntax-highlighter enhancement(artifact): improve search results enhancement(artifact): add metadata accordion fix: use paths constant for route fix: lazy load artifacts chore: remove accordion, use content for titles fix: add type for artifact uri fix: fallback for uri type assertion chore: remove gcTime setting fix: remove unnecessary query dependency
fix: reusable handlers fix: lint fix: rm dummy artifact uri
feat(artifacts): add verification hook feat(artifacts): add attestations tab
Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> fix(branding): update favicon fix(branding): update favicon chore(artifacts): add attestation component, update hook chore: add mutation hook and mock data add attestation results
3496e5e to
8864666
Compare
…tificateChain data from signature, implemnted sharedFunction for code block actions, added one more signature to mock data
carlosthe19916
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kahboom thanks for the PR.
I have reviewed this PR just in general terms. And added a couple of comments.
I was not able to make it work due to an error. But we can iterate it title by title
{
"details": {
"error": "error getting simple signing layer: error getting signature manifest: GET https://index.docker.io/v2/library/nginx/manifests/sha256-553f64aecdc31b5bf944521731cd70e35da4faed96b2b7548a3d8e2598c52a42.sig: MANIFEST_UNKNOWN: manifest unknown; unknown tag=sha256-553f64aecdc31b5bf944521731cd70e35da4faed96b2b7548a3d8e2598c52a42.sig"
},
"verified": false
}
client/src/app/queries/helpers.ts
Outdated
| }, | ||
| }); | ||
| }; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally speaking Mutations involve create/update/delete data performing server side-effects. Therefore I think there is no need to mock mutations.
The useMockableQuery was originally designed as a way or providing data so we can demo things without the need to wait for backend features and we can work in parallel.
I am inclined to mock things for read only purposes and don't mock things for create/update/delete; we will eventually make our code more complex without having too much gain in return.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point about mocking things for CRUD. I was thinking more about having it still work with static pages but this could be the point of diminishing returns.
client/src/app/queries/artifacts.ts
Outdated
| // - TData: ArtifactVerificationViewModel → what the UI consumes | ||
| // - TError: AxiosError<_Error> → error shape from the SDK | ||
| // - TVariables: IVerifyArtifactVariables → what the caller passes in (uri, expectedSAN, etc.) | ||
| return useMockableMutation<ArtifactVerificationViewModel, AxiosError<_Error>, IVerifyArtifactVariables>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure we we should use Mutations here. As mentioned in other comments here Mutations are for Create/Update/Delete of data. Seems to me that although we are using a POST HTTP request, the operation is a READ operation.
Generally speaking we should use:
useQuery: for READ operations. Those operations are idempotent. Usually used on GET HTTP requests. However, POST requests can also serve READ operations so we can use POST here too.useMutation: for CREATE/UPDATE/DELETE data in the backend. Usually done by POST, PUT, DELETE. this is not an idempotent operation because we are changing data in the server side.
I might be wrong but I think we are doing a READ operation and we could use useQuery then combine it with POST, what do you think?
Note: I might be wrong in regards of this being only a READ operation though
| return ( | ||
| <div style={{ margin: "2em auto" }}> | ||
| <p>Showing 1 of 1</p> | ||
| <Card style={{ margin: "1.5em auto 2em", overflowY: "hidden" }}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In multiple lines we are using style and Content H4. I would advocate for using the defaults as much as possible. So leave the default sizes of all components and only change the defaults by strict request of UX. It will help us maintain visual consistency with all pages in the long term
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure thing!
…stations compact, change the way attestation content look
…tions extract RekorEntryPanel into a separate reusable component, made atte…
Hey @stanislavsemeniuk , I'm leaving this as a draft PR until we have something usable. These are just dummy components most of them copy & pasted straight from PatternFly to demonstrate the idea. We need to update the code to use the API data, where possible. If we don't expect the data anytime soon we can remove it for now.
We have an OpenAPI spec that gets updated when the relevant part of the API gets updated, and it's located here: https://github.com/securesign/rhtas-console-ui/blob/main/client/openapi/console.yaml
We have a manually-defined SDK file that refers to that spec for queries, it gets created locally at
client/src/app/client/sdk.gen.ts.Summary by Sourcery
Add a verification interface to the artifact details page, showcasing metadata summary cards and an interactive list of signatures with expandable details.
New Features:
Enhancements:
Chores: